Fix initializer load for in-memory external data#29349
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes Initializer(graph, ...) behavior for initializers that use in-memory external data when the corresponding OrtValue is not registered: the constructor no longer incorrectly requires a non-empty model_path, and instead falls back to CreateTensorFromTensorProto to load from the in-memory external buffer (consistent with the other Initializer constructor). This addresses the CreateSessionFromArray failure reported in #28267.
Changes:
- Remove the
model_path must not be emptyenforcement in the graph-basedInitializerconstructor for the in-memory-external-data + missing-OrtValue case. - Add a regression test covering an in-memory external-data initializer with empty
model_pathand no registeredOrtValue.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| onnxruntime/core/optimizer/initializer.cc | Removes the incorrect model_path enforcement so in-memory external data can be loaded via CreateTensorFromTensorProto when no OrtValue is present. |
| onnxruntime/test/optimizer/initializer_test.cc | Adds a regression test to ensure in-memory external-data initializers load correctly without model_path when no OrtValue is registered. |
|
@tianleiwu Could you please take a look? Thanks. |
tairenpiao
force-pushed
the
fix-initializer-inmemory-from-array
branch
from
e2a7d88 to
a9143b4
Compare
tianleiwu
left a comment
tianleiwu
left a comment
There was a problem hiding this comment.
Verdict: Approve
The fix is correct and well-scoped. The removed ORT_ENFORCE(!model_path.empty()) was reached only inside the if (HasExternalDataInMemory(tensor_proto)) branch. HasExternalDataInMemory() is true only when the external-data location is a memory-address tag, and in that case CreateTensorFromTensorProto -> TensorProtoToTensor -> GetExtDataFromTensorProto resolves the data directly from the embedded address and copies it via MakeCpuTensorCopy, so model_path is genuinely never needed. Falling through to that path is the right fix for #28267.
Verified:
- File-based external data (
HasExternalDataInMemory == false) is unaffected — the graph constructor never enforcedmodel_pathfor that case, so no regression. - A copy is made, so there is no dangling concern after construction even when the marker points into the caller's model-bytes buffer.
- Dropping the
#if !defined(__wasm__)guard is safe: on wasm the enforce never ran anyway. - The added test includes are direct (good hygiene).
One minor test suggestion inline.
| const Initializer init(graph, tensor_proto, std::filesystem::path{}, /*check_outer_scope=*/false); | ||
|
|
||
| ASSERT_EQ(init.size(), backing.size()); | ||
| const auto values = init.DataAsSpan<float>(); |
There was a problem hiding this comment.
Nitpick: the core contract of this constructor path is that it copies the in-memory data (MakeCpuTensorCopy). Consider also asserting copy-independence — e.g. mutate backing after constructing init and confirm the values read from init are unchanged. That would lock in the copy semantics and guard against a future regression to aliasing the source buffer.
Description
The
Initializer(graph, …)constructor wrongly required amodel_pathfor an initializer with in-memory external data when itsOrtValuewasn't registered. It now loads the data from memory viaCreateTensorFromTensorProto, matching the other constructor.Motivation and Context
Fixes #28267. A model loaded from bytes via
CreateSessionFromArrayfailed withmodel_path must not be empty; loading the same model from a file worked.